test: add plugin routing tests for bundled irssi plugin#352
Conversation
Covers namespace passthrough routing and doctor dependency health for the irssi passthrough plugin, mirroring existing plugin test patterns (eza). Guards against regressions in passthrough command resolution and binary-check reporting.
Covers namespace passthrough routing and doctor dependency health for the dar passthrough plugin, guarding passthrough command resolution and binary-check reporting.
Covers namespace passthrough routing and doctor dependency health for the xata passthrough plugin, guarding passthrough command resolution and binary-check reporting.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Wait—let me reconsider the actual instruction. The verification gate passed, but this change may not address the stated objective. Review before merging. |
|
🏛️ Automaintainer architecture review: VIOLATION The change adds new test files under The change passed verification, but it violates this repo's architecture rules. The PR was set to draft and auto-merge is held — review before marking it ready. |
📝 WalkthroughWalkthroughAdds Jest CLI integration tests for the dar, irssi, and xata plugins, covering namespace passthrough and healthy binary reporting from ChangesPlugin CLI test coverage
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Automaintainer Review Verdict: APPROVED — Adds test coverage for irssi, dar, and xata passthrough plugins following the established pattern (mirrors eza/clix tests). Changes are purely additive (3 new test files, no production code touched), use proper temp-dir isolation, and clean up in afterAll. CI is pending but not failing. The automated 'off-objective' and 'architecture violation' comments are false positives: the PR focus is valid test maintenance and tests/ is the established convention across all open PRs in this batch. |
|
Approved by automaintainer review team — ready for human merge |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
__tests__/dar-plugin.test.js (1)
8-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract duplicate test utilities to a shared helper.
The
runNoServerandwriteFake*Binaryfunctions are identical across thedar,irssi, andxataplugin tests. Extracting them into a shared utility file will adhere to DRY principles and simplify test maintenance.
__tests__/dar-plugin.test.js#L8-L38: remove these functions and import them from a shared helper.__tests__/irssi-plugin.test.js#L8-L38: remove these functions and import them from a shared helper.__tests__/xata-plugin.test.js#L8-L38: remove these functions and import them from a shared helper.♻️ Proposed shared helper
For example, you could create a
__tests__/test-utils.jsfile:const fs = require("fs") const path = require("path") const { execSync } = require("child_process") const CLI = path.join(__dirname, "..", "cli", "supercli.js") function runNoServer(args, options = {}) { try { const env = { ...process.env } delete env.SUPERCLI_SERVER const out = execSync(`node "${CLI}" ${args}`, { encoding: "utf-8", timeout: 15000, env: { ...env, ...(options.env || {}) } }) return { ok: true, output: out.trim(), code: 0 } } catch (err) { return { ok: false, output: (err.stdout || "").trim(), stderr: (err.stderr || "").trim(), code: err.status } } } function writeFakeBinary(dir, name, version) { const bin = path.join(dir, name) fs.writeFileSync(bin, [ "#!/usr/bin/env node", "const args = process.argv.slice(2);", `if (args.includes('--version')) { console.log('${name} ${version}-test'); process.exit(0); }`, "console.log(JSON.stringify({ ok: true, args }));" ].join("\n"), "utf-8") fs.chmodSync(bin, 0o755) return bin } module.exports = { runNoServer, writeFakeBinary }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/dar-plugin.test.js` around lines 8 - 38, Extract the duplicated runNoServer and writeFake*Binary utilities into a shared __tests__/test-utils.js helper, exporting runNoServer and a parameterized writeFakeBinary. In __tests__/dar-plugin.test.js lines 8-38, __tests__/irssi-plugin.test.js lines 8-38, and __tests__/xata-plugin.test.js lines 8-38, remove the local implementations and import the shared utilities, updating each call to pass the binary name and version as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@__tests__/dar-plugin.test.js`:
- Around line 12-16: Quote the CLI path in the execSync command constructing the
node invocation so paths containing spaces execute correctly. Update the command
in __tests__/dar-plugin.test.js lines 12-16, __tests__/irssi-plugin.test.js
lines 12-16, and __tests__/xata-plugin.test.js lines 12-16; each site requires
the same change to the CLI interpolation.
- Line 44: Use Node’s path.delimiter when constructing PATH in the test
environment setup, replacing the hardcoded colon in __tests__/dar-plugin.test.js
lines 44-44, __tests__/irssi-plugin.test.js lines 44-44, and
__tests__/xata-plugin.test.js lines 44-44. Ensure each file imports or accesses
the path module and prepends fakeDir with the platform-specific delimiter while
preserving the existing fallback path.
---
Nitpick comments:
In `@__tests__/dar-plugin.test.js`:
- Around line 8-38: Extract the duplicated runNoServer and writeFake*Binary
utilities into a shared __tests__/test-utils.js helper, exporting runNoServer
and a parameterized writeFakeBinary. In __tests__/dar-plugin.test.js lines 8-38,
__tests__/irssi-plugin.test.js lines 8-38, and __tests__/xata-plugin.test.js
lines 8-38, remove the local implementations and import the shared utilities,
updating each call to pass the binary name and version as needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47d1c778-81e7-43b5-b9c6-682ca2b2c651
📒 Files selected for processing (3)
__tests__/dar-plugin.test.js__tests__/irssi-plugin.test.js__tests__/xata-plugin.test.js
| const out = execSync(`node ${CLI} ${args}`, { | ||
| encoding: "utf-8", | ||
| timeout: 15000, | ||
| env: { ...env, ...(options.env || {}) } | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Quote the CLI path to prevent execution failures.
If the repository is cloned into a directory with spaces in its name, execSync will fail because the shell will improperly split the unquoted ${CLI} path. Wrap ${CLI} in quotes to ensure it resolves correctly.
__tests__/dar-plugin.test.js#L12-L16: change`node ${CLI} ${args}`to`node "${CLI}" ${args}`.__tests__/irssi-plugin.test.js#L12-L16: apply the same quoting to theexecSynccall.__tests__/xata-plugin.test.js#L12-L16: apply the same quoting to theexecSynccall.
💻 Proposed fix
- const out = execSync(`node ${CLI} ${args}`, {
+ const out = execSync(`node "${CLI}" ${args}`, {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const out = execSync(`node ${CLI} ${args}`, { | |
| encoding: "utf-8", | |
| timeout: 15000, | |
| env: { ...env, ...(options.env || {}) } | |
| }) | |
| const out = execSync(`node "${CLI}" ${args}`, { | |
| encoding: "utf-8", | |
| timeout: 15000, | |
| env: { ...env, ...(options.env || {}) } | |
| }) |
🧰 Tools
🪛 OpenGrep (1.25.0)
[ERROR] 12-16: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
📍 Affects 3 files
__tests__/dar-plugin.test.js#L12-L16(this comment)__tests__/irssi-plugin.test.js#L12-L16__tests__/xata-plugin.test.js#L12-L16
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@__tests__/dar-plugin.test.js` around lines 12 - 16, Quote the CLI path in the
execSync command constructing the node invocation so paths containing spaces
execute correctly. Update the command in __tests__/dar-plugin.test.js lines
12-16, __tests__/irssi-plugin.test.js lines 12-16, and
__tests__/xata-plugin.test.js lines 12-16; each site requires the same change to
the CLI interpolation.
| const fakeDir = fs.mkdtempSync(path.join(os.tmpdir(), "dcli-dar-")) | ||
| const tempHome = fs.mkdtempSync(path.join(os.tmpdir(), "dcli-home-dar-")) | ||
| writeFakeDarBinary(fakeDir) | ||
| const env = { ...process.env, PATH: `${fakeDir}:${process.env.PATH || ""}`, SUPERCLI_HOME: tempHome } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use path.delimiter for cross-platform PATH compatibility.
Hardcoding : as the PATH separator breaks the test environment on Windows, which uses ;. Use Node's built-in path.delimiter to safely prepend the fake directory to the path.
__tests__/dar-plugin.test.js#L44-L44: replace${fakeDir}:${process.env.PATH || ""}with${fakeDir}${path.delimiter}${process.env.PATH || ""}.__tests__/irssi-plugin.test.js#L44-L44: replace the hardcoded:separator with${path.delimiter}.__tests__/xata-plugin.test.js#L44-L44: replace the hardcoded:separator with${path.delimiter}.
💻 Proposed fix
- const env = { ...process.env, PATH: `${fakeDir}:${process.env.PATH || ""}`, SUPERCLI_HOME: tempHome }
+ const env = { ...process.env, PATH: `${fakeDir}${path.delimiter}${process.env.PATH || ""}`, SUPERCLI_HOME: tempHome }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const env = { ...process.env, PATH: `${fakeDir}:${process.env.PATH || ""}`, SUPERCLI_HOME: tempHome } | |
| const env = { ...process.env, PATH: `${fakeDir}${path.delimiter}${process.env.PATH || ""}`, SUPERCLI_HOME: tempHome } |
📍 Affects 3 files
__tests__/dar-plugin.test.js#L44-L44(this comment)__tests__/irssi-plugin.test.js#L44-L44__tests__/xata-plugin.test.js#L44-L44
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@__tests__/dar-plugin.test.js` at line 44, Use Node’s path.delimiter when
constructing PATH in the test environment setup, replacing the hardcoded colon
in __tests__/dar-plugin.test.js lines 44-44, __tests__/irssi-plugin.test.js
lines 44-44, and __tests__/xata-plugin.test.js lines 44-44. Ensure each file
imports or accesses the path module and prepends fakeDir with the
platform-specific delimiter while preserving the existing fallback path.
1 participant
Automated maintenance run by automaintainer.
Focus:
WORK ALREADY IN FLIGHT — do not overlap:
These automaintainer pull requests are already open and awaiting review. Do NOT re-implement, refactor, or restructure the files they touch — pick non-overlapping work, and never recreate a change an open PR already makes. If your objective unavoidably overlaps one of these, choose a different, complementary improvement instead.
touches: tests/httprobe-plugin.test.js, tests/lazysql-plugin.test.js, tests/sq-plugin.test.js
touches: tests/run-command.test.js, tests/skills-mcp.test.js
touches: tests/chainloop-plugin.test.js, tests/overmind-plugin.test.js, tests/resvg-plugin.test.js
touches: tests/amass-plugin.test.js, tests/goss-plugin.test.js, tests/hivemind-plugin.test.js, tests/mods-plugin.test.js, tests/tlrc-plugin.test.js
touches: tests/komiser-plugin.test.js, tests/upx-plugin.test.js, tests/wtfutil-plugin.test.js
touches: tests/coder-plugin.test.js, tests/pkl-plugin.test.js, tests/rbspy-plugin.test.js
touches: tests/diun-plugin.test.js, tests/notion-cli-plugin.test.js, tests/slim-plugin.test.js
touches: tests/help.test.js, tests/mcp-diagnostics.test.js, package.json
Branch:
am/am-f17c27-ti5qm6Summary
Add plugin routing test coverage for three bundled passthrough plugins that previously had none: irssi, dar, and xata. Each test suite installs the plugin into an isolated temp SUPERCLI_HOME with a fake binary on PATH, then verifies namespace passthrough command resolution (command name + forwarded args) and that
plugins doctorreports the required binary as healthy. This mirrors the existing eza/clix plugin test patterns and guards against regressions in passthrough routing and binary-check reporting.Diff:
Summary by CodeRabbit